feat: add rpc filter for early rejection of configured method-param t… - #64
feat: add rpc filter for early rejection of configured method-param t…#64notwedtm wants to merge 1 commit into
Conversation
Mctursh
left a comment
There was a problem hiding this comment.
Went through this pretty carefully. The engine itself is great, matching's exact and order-independent, batch keeps its indexes straight, the hot path's basically free when nothing's configured, and the shared YAML plays nice with the ingestor. No notes there.
What kept catching me is the rejection path rolling its own HTTP + JSON-RPC error shape when the server already has conventions for the same cases, so most of these are really "reuse what's there?" questions:
-
-32601is what the server already returns for unknown methods (tests assert on it), so a blocked call looks identical by code to a method that doesn't exist. The commitment=processed path already does the right thing for "method's fine, param isn't",-32602+ data. Filter could match that. -
The 405 is always on, even with emit_http_errors off, which cuts against the "HTTP status behavior" section that says client errors stay 200. And in a batch, one filtered item flips the whole batch to 405 while the other results are sitting right there in the body.
-
One real bug: an empty
--config/SUPERBANK_CONFIGcrash-loops on boot (clap hands you Some(""), not None), where the other path flags guard the empty case.
Minor: a method-only entry only matches params: [], so [getHealth] won't catch a no-arg getHealth that omits params. Documented, just easy to trip on.
Nothing blocking really, mostly the reuse questions plus that config fix. Nice feature to have.
| const JSON_RPC_INTERNAL_ERROR_CODE: i64 = -32603; | ||
| const JSON_RPC_REQUEST_TIMEOUT_CODE: i64 = -32000; | ||
| const JSON_RPC_REQUEST_TIMEOUT_MESSAGE: &str = "Request timeout"; | ||
| const JSON_RPC_METHOD_NOT_ALLOWED_CODE: i32 = -32601; |
There was a problem hiding this comment.
This reuses -32601, which is JSON-RPC's "Method not found" and the same code the server already returns for genuinely-unknown methods (the fallback arm in dispatch, and tests/mod.rs asserts code == -32601). So a client keying on the code can't tell a blocked (method, params) from a method that doesn't exist. Only the message text and the HTTP status differ.
The repo already handles this exact shape (method is valid, a specific param value is rejected by server policy): the commitment=processed rejection in signatures.rs and blocks.rs returns -32602 "Only confirmed or finalized commitments are supported" with data: {requestedCommitment}. Could the filter match that, -32602 + data describing the matched method/params, or take a dedicated code in the -32000..-32099 server range if it should read as "server-imposed"?
| JSON_RPC_METHOD_NOT_ALLOWED_MESSAGE, | ||
| None, | ||
| ); | ||
| *response.status_mut() = StatusCode::METHOD_NOT_ALLOWED; |
There was a problem hiding this comment.
This sets 405 unconditionally. Everywhere else the server treats HTTP error statuses as opt-in via emit_http_errors (default false means everything is HTTP 200; only four server-side codes ever promote, to 503). The README's "HTTP status behavior" section says it directly: "Client, malformed-request, and data-condition errors remain HTTP 200 OK." A filtered request is a client error by that taxonomy but returns 405 regardless of the flag, so that section and the filter section now disagree.
Should the 405 be gated behind emit_http_errors (200 by default, like every other client error) so the "HTTP errors are opt-in" contract holds? If the 405 is intentional for ops visibility, maybe a one-line exception in the HTTP-status section so the two don't read as contradictory.
| let mut response = Json(Value::Array(response_values)).into_response(); | ||
| if parameter_filter_matched { | ||
| *response.status_mut() = StatusCode::METHOD_NOT_ALLOWED; |
There was a problem hiding this comment.
One filtered item sets the whole batch's HTTP status to 405, while response_values still carries every other item's successful result. A client that checks resp.ok before parsing (the exact client the opt-in 503 mechanism exists for) treats the succeeded siblings as failed and may drop or retry the whole batch. JSON-RPC models a batch as N independent responses, so one administratively blocked item escalating the transport status for all of them is a sharp edge, and unlike the 503 batch-promotion it isn't gated by emit_http_errors.
Related: if emit_http_errors is on and a batch has both a filtered item and a 503-eligible error, promote_http_status_for_json_rpc_errors overwrites this 405 with 503 unconditionally, so the documented "returns 405" doesn't hold in that combo.
| pub async fn run_server(args: RpcConfig) -> RpcResult<()> { | ||
| info!("Starting Solana RPC server on {}:{}", args.host, args.port); | ||
| let rpc_parameter_filters = | ||
| RpcParameterFilterSet::load(args.config.as_deref()).map_err(RpcError::Config)?; |
There was a problem hiding this comment.
config is Option<PathBuf> passed straight to load() with no empty guard, and clap turns SUPERBANK_CONFIG="" (or --config "") into Some("") (checked on 4.6.0). So an empty placeholder reads "" and fails boot instead of no-op'ing as "unset." The other optional path flags guard this (disk_cache_path, dragonsmouth_endpoint use .map(str::trim).filter(|v| !v.is_empty())). Same idea here:
RpcParameterFilterSet::load(
args.config.as_deref().filter(|p| !p.as_os_str().is_empty()),
)| if method.trim().is_empty() { | ||
| return Err(Self::entry_error(path, index, "method must not be empty")); | ||
| } | ||
|
|
||
| let candidates = filters | ||
| .by_method_and_arity | ||
| .entry(method) |
There was a problem hiding this comment.
Small one: the loader validates method.trim().is_empty() but then stores the untrimmed method as the key. A quoted entry with a stray space (["getThing ", ...]) passes validation, counts in len(), and logs as loaded, but never matches, since matches compares the raw request method. Narrow case (unquoted YAML strips the space), but it's a silent no-op that slips past the loader's otherwise fail-loud validation. Trimming before storing, or rejecting a non-trimmed method like the empty case, keeps it fail-loud.
This pull request introduces an exact method and parameter filtering mechanism for the
superbank-rpcserver, allowing requests to be rejected before reaching handlers or consuming resources. The feature is configured via a shared YAML file and is integrated throughout the codebase, including configuration, server startup, request handling, and tests.The most important changes are:
Parameter Filtering Feature:
RpcParameterFilterSet(incrates/superbank-rpc/src/request_filter.rs) that loads method/parameter filters from a YAML config file, provides efficient matching, and rejects requests that match any filter entry. Includes comprehensive tests for filter logic and config parsing.RpcConfigto accept a--configCLI argument orSUPERBANK_CONFIGenvironment variable, specifying the shared YAML file for filters. Added parsing tests. [1] [2]AppState, with logging of the number of loaded filters. [1] [2] [3] [4] [5]Request Handling Logic:
Documentation:
README.mdandcrates/superbank-rpc/README.mdto document the new filter configuration, YAML syntax, matching semantics, and error behavior. [1] [2]Testing and Test Utilities:
AppStateconstruction, ensuring coverage of the new filtering logic. [1] [2] [3] [4] [5] [6] [7]With these changes,
superbank-rpccan efficiently reject unwanted method/parameter combinations at the entry point, improving security and resource utilization.…uples